Support uses-permission merging in manifest merger #13445
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Bencodes
force-pushed
the
support-uses-permissions-merging
branch
7 times, most recently
from
345e7e6
to
d0378fe
Compare
Bencodes
commented
May 13, 2021
| @@ -176,8 +187,7 @@ private static Path removePermissions(Path manifest, Path outputDir) | |||
| } | |||
| } | |||
| // Write resulting manifest to the output directory, maintaining full path to prevent collisions | |||
| Path output = outputDir.resolve(manifest.toString().replaceFirst("^/", "")); | |||
There was a problem hiding this comment.
This was failing on windows machines causing the original files to be modified in place.
There was a problem hiding this comment.
It will be easier to import this PR if we can evaluate this change separately. Is it right that this was an existing problem and not needed for merging permissions?
There was a problem hiding this comment.
Correct. I can pull this out into another PR that can be imported independently of this, but this PR will need to be rebased against that for the tests to pass.
There was a problem hiding this comment.
Pulled this out into #13760
There was a problem hiding this comment.
Hi Ben, if you've forked the Windows-related fixes into a separate PR, would you mind amending this PR to only include the changes for permission merging?
There was a problem hiding this comment.
@ted-xie The tests for this wont pass on windows until the other PR lands. I'll rebase this once that lands.
|
@ahumesky mind taking a look at this? |
|
@ahumesky can you help take a look? |
Bencodes
force-pushed
the
support-uses-permissions-merging
branch
from
4024980
to
35bd48c
Compare
Bencodes
force-pushed
the
support-uses-permissions-merging
branch
2 times, most recently
from
dec1425
to
4191be0
Compare
Bencodes
force-pushed
the
support-uses-permissions-merging
branch
from
4191be0
to
7c584d1
Compare
ted-xie
pushed a commit
to ted-xie/bazel
that referenced
this pull request
Jul 6, 2022
Adding support for conditionally merging `uses-permissions`. bazelbuild#10628 bazelbuild#5411 Closes bazelbuild#13445. RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag. PiperOrigin-RevId: 439613035
ted-xie
pushed a commit
to ted-xie/bazel
that referenced
this pull request
Jul 6, 2022
Adding support for conditionally merging `uses-permissions`. bazelbuild#10628 bazelbuild#5411 Closes bazelbuild#13445. RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag. PiperOrigin-RevId: 439613035
ted-xie
pushed a commit
to ted-xie/bazel
that referenced
this pull request
Jul 6, 2022
Adding support for conditionally merging `uses-permissions`. bazelbuild#10628 bazelbuild#5411 Closes bazelbuild#13445. RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag. PiperOrigin-RevId: 439613035
ckolli5
pushed a commit
that referenced
this pull request
Jul 6, 2022
* Support uses-permission merging in manifest merger Adding support for conditionally merging `uses-permissions`. #10628 #5411 Closes #13445. RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag. PiperOrigin-RevId: 439613035 * Make ManifestMergerAction worker compatible Calling `System#exit` kills the worker during the build. Passing the exception up to the worker should be enough for it to end up in the worker or local execution output. Closes #14427. PiperOrigin-RevId: 447808701 * Fix ManifestMergerAction test case on windows `manifest.toString().replaceFirst("^/", "")` silently fails on windows machines causing `removePermissions` to write to the original test file. This pull request creates a new temp file that `removePermissions` can write the modified manifest to. Pulling this change out of another PR so that it's easier to merge. Original PR here https://github.com/bazelbuild/bazel/pull/13445/files#r631575251 Closes #13760. PiperOrigin-RevId: 438643774 Co-authored-by: Ben Lee <ben@ben.cm>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding support for conditionally merging
uses-permissions.#10628
#5411